White flag #1806

acha-bill · 2020-03-20T16:00:38Z

Description

Ability to confirm conflicting bundle while applying only the first to the ledger.
The order of bundles is determined by a topological sort via DFS preferring trunk first.

Fixes #1684

Type of change

Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

Existing unit and regression tests pass
Added new regression test where a milestone approves 2 double-spend bundles and asserts that even though only 1 affects the ledger, they are both confirmed.

Checklist:

My code follows the style guidelines for this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

GalRogozinski · 2020-03-22T13:46:33Z

src/main/java/com/iota/iri/service/ledger/impl/LedgerServiceImpl.java

-                        return null;
-                    }
-                    if (!milestoneService.isTransactionConfirmed(transactionViewModel, milestoneIndex)) {
+        Stack<Hash> stack = new Stack<>();


Please implement stack with a Deque
It is a much more efficient implementation...
Stack relies on Vector which is quite obsolete

See the javadocs of the Stack class:

* <p>A more complete and consistent set of LIFO stack operations is * provided by the {@link Deque} interface and its implementations, which * should be used in preference to this class. For example: * <pre> {@code * Deque<Integer> stack = new ArrayDeque<Integer>();}</pre>

GalRogozinski · 2020-03-22T14:47:42Z

src/main/java/com/iota/iri/service/ledger/impl/LedgerServiceImpl.java

+                            if(isConsistent){
+                                state = currentState;
+                            }else{
+                                transactionViewModel.isConflicting(tangle, initialSnapshot, true);


Can you change to setConflicting

is is the prefix of a boolean java getter

GalRogozinski · 2020-03-22T15:50:37Z

src/main/java/com/iota/iri/service/ledger/impl/LedgerServiceImpl.java

+                                    final Hash address = bundleTransactionViewModel.getAddressHash();
+                                    final Long value = currentState.get(address);
+                                    currentState.put(address, value == null ? bundleTransactionViewModel.value()
+                                            : Math.addExact(value, bundleTransactionViewModel.value()));


If an overflow exception is thrown here then we would like to make sure the bundle is ignored

GalRogozinski · 2020-03-22T18:02:04Z

src/main/java/com/iota/iri/model/persistables/Transaction.java

+    /**
+     * This flag indicates whether the transaction is conflicting and was ignored in balance computation
+     */
+    public boolean conflicting = false;


Since you branched off dev this is correct
But we know that soon this should turn Atomic or volatile
So might as well do it now so we won't forget...

Technically volatile suffices here but since in 1.9.0 we made everything Atomic we made as well make it atomic

GalRogozinski · 2020-03-22T19:14:27Z

src/main/java/com/iota/iri/service/ledger/impl/LedgerServiceImpl.java

+
+                boolean approvedTrunk = (trunk.snapshotIndex() > 0) && (trunk.snapshotIndex() != milestoneIndex);
+                boolean approvedBranch = (branch.snapshotIndex() > 0) && (branch.snapshotIndex() != milestoneIndex);
+                if ((trunk.isEmpty()  || visitedTransactions.contains(trunk.getHash()) || approvedTrunk) &&


One should note that an empty transaction is the same as the genesis transaction (genesis transaction has all 9's trytes, and all 9s Hash). Then it is fine...

If the transaction was empty because the trunk tx is simply missing (not solid) then we have a problem...

I know you went according to the RFC, I just added a comment on it as well...

GalRogozinski · 2020-03-22T19:22:14Z

src/main/java/com/iota/iri/controllers/TransactionViewModel.java

+    /**
+     * Checks if a transaction is empty.
+     * @return True if empty. False otherwise.
+     */
+    public boolean isEmpty(){
+        return Arrays.equals(getBytes(), new byte[SIZE]);
+    }
+


Currently we are just check for TransactionViewModel.PREFILLED_SLOT
I am not saying that this is smart (I didn't make this weird design)

But it will be more awful if we have more than one way to do this check...
So I wouldn't have this method now

DyrellC

Just the one comment about labelling/commenting, otherwise it looks like it passes muster. The updated regression test is a nice touch as well.

DyrellC · 2020-03-24T05:45:16Z

src/main/java/com/iota/iri/service/ledger/impl/LedgerServiceImpl.java

+
+                boolean approvedTrunk = (trunkTransactionViewModel.snapshotIndex() > 0) && (trunkTransactionViewModel.snapshotIndex() != milestoneIndex);
+                boolean approvedBranch = (branchTransactionViewModel.snapshotIndex() > 0) && (branchTransactionViewModel.snapshotIndex() != milestoneIndex);
+                if ((trunkTransactionViewModel.getType() == TransactionViewModel.PREFILLED_SLOT  || visitedTransactions.contains(trunkTransactionViewModel.getHash()) || approvedTrunk) &&


Perhaps providing labels for these conditions as booleans instead of rewriting them in the if and else statements here could help to declutter this section. There are a lot of conditions being thrown around very close together here that could make it harder to follow. For example another boolean for each the trunk and branch types to be reused below in the else statement. More descriptive labels or a few extra comments could go a long way towards making the logic easier for others to follow.

GalRogozinski

Some more changes please

GalRogozinski · 2020-03-25T11:30:40Z

src/main/java/com/iota/iri/service/ledger/impl/LedgerServiceImpl.java

+                boolean isLeafTrunk = isEmptyTrunk  || visitedTransactions.contains(trunkTransactionViewModel.getHash()) || isApprovedTrunk;
+                boolean isLeafBranch = isEmptyBranch || visitedTransactions.contains(branchTransactionViewModel.getHash()) || isApprovedBranch;


hmm I think that here we let way for confirming non solids...
I am pretty sure that if we reach an empty trunk it means we have a non-valid subtangle

The exception here is that if it is the genesis transaction, then it should be fine (but tip-sel should not allow it and we should test for it)

GalRogozinski · 2020-03-25T13:50:37Z

python-regression/tests/features/machine3/3_transaction_tests.feature

            | keys   | values | type          |
            | states | False  | boolListMixed |

+    Scenario: Conflicting bundles are ignored


Maybe add a scenario for invalid bundles

Already done in my PR, theyre in the disabled regression tests file in machine 3

was just about to tag you here @kwek20 but I see you are very alert :-)

GalRogozinski · 2020-03-31T14:19:02Z

src/main/java/com/iota/iri/service/ledger/impl/LedgerServiceImpl.java

+                //Don't confirm non-solid txs.
+                Hash genesisHash = Hash.NULL_HASH;
+                if(isEmptyTrunk  && !allowGenesisReference){
+                    return null;
+                 }
+                //proceed only if empty trunk or branch is genesis
+                if((isEmptyTrunk && !trunkTransactionViewModel.getHash().equals(genesisHash))||
+                (isEmptyBranch && !branchTransactionViewModel.getHash().equals(genesisHash))){
+                    return null;
+                }
+


In the event that trunk or branch have a non 9s hash then this is wrong

I believe (I think I missed it in the last review so my bad) that the 9s genesis hash is in the solidEntryPointsSet

So I think the flag is unnecessary (because we should always allow genesis reference)
I think we should always return null if it is PREFILLED_SLOT

Since we return null if branch or trunk is PREFILLED_SLOT.
Is this the only definition of a leaf?

boolean isLeafTrunk = visitedTransactions.contains(trunkTransactionViewModel.getHash()) || isApprovedTrunk;

Yes, when visitedTransactions contain all SEPs and genesis

GalRogozinski

I was having one last look before publishing this on Discord.

I think you can still take the opportunity to:

Extract private methods to make generateBalanceDiff more readable
Add a unit test that mocks a tangle with conflicts and generates the balance diff

No need to rush to do it this sprint because we might get external feedback

GalRogozinski · 2020-04-06T08:58:20Z

src/main/java/com/iota/iri/service/ledger/impl/LedgerServiceImpl.java

+                }else if(!isLeafBranch) {
+                    stack.addFirst(branchTransactionViewModel.getHash());
+                }
+            }catch(Exception e){


Suggested change

}catch(Exception e){

} catch(Exception e) {

acha-bill added 2 commits March 20, 2020 17:06

white-flag

80655fa

added check if tvm is empty

d05b6bc

acha-bill force-pushed the white-flag branch from a3256f2 to d05b6bc Compare March 20, 2020 16:42

GalRogozinski suggested changes Mar 22, 2020

View reviewed changes

GalRogozinski requested review from DyrellC and kwek20 March 22, 2020 19:23

acha-bill added 2 commits March 23, 2020 12:40

refactoring

0a70b70

fix milestone issuance error

7c430ce

DyrellC approved these changes Mar 24, 2020

View reviewed changes

refactor: extra complicated conditions to variables

6a2653f

GalRogozinski suggested changes Mar 25, 2020

View reviewed changes

do not confirm non-solid txs

a0532cb

acha-bill force-pushed the white-flag branch from 89a9c48 to a0532cb Compare March 31, 2020 10:55

GalRogozinski suggested changes Mar 31, 2020

View reviewed changes

acha-bill and others added 2 commits April 1, 2020 15:46

remove geneis reference flag and isempty check

2d4cc70

Merge branch 'white-flag' into white-flag

192670c

GalRogozinski reviewed Apr 6, 2020

View reviewed changes

		boolean isLeafTrunk = isEmptyTrunk \|\| visitedTransactions.contains(trunkTransactionViewModel.getHash()) \|\| isApprovedTrunk;
		boolean isLeafBranch = isEmptyBranch \|\| visitedTransactions.contains(branchTransactionViewModel.getHash()) \|\| isApprovedBranch;

White flag #1806

Are you sure you want to change the base?

White flag #1806

Uh oh!

Conversation

acha-bill commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DyrellC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GalRogozinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GalRogozinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

acha-bill commented Mar 20, 2020 •

edited

Loading